WIP: Server rotate key (for reference) #534
Conversation
…role. Signed-off-by: Ying Li <[email protected]>
…d a RotateKeyHandler. Signed-off-by: Ying Li <[email protected]>
…sn't going its own unique version Signed-off-by: David Lawrence <[email protected]> (github: endophage)
…root. This also factors out some share code between the snapshot generation handler and the snapshot generation in the validator. Signed-off-by: Ying Li <[email protected]>
…motely. Signed-off-by: Ying Li <[email protected]>
Signed-off-by: Ying Li <[email protected]>
| // GUN and role. If no keys exist for the GUN+role, returns an ErrNoKeys. | ||
| GetLatestKey(gun, role string) (*ManagedPublicKey, error) | ||
|
|
||
| // HasAnyKeys returns true if any non-expired keys exist for the given GUN, |
There was a problem hiding this comment.
Only necessary because we validate that the root.json has timestamp keys that the server has. If we want to eliminate this requirement and allow users to manage their own timestamp keys, we can probably eliminate this interface.
It is also used as a shortcut to see if we can generate a snapshot, but we can probably just try to generate one and then fail if we don't have the keys.
|
I need to think about this more. I really like the naive implementation of just returning a new key whenever the user asks for it and letting them sign it in as and when they feel like it. That would completely remove the need for the (unfortunately named) |
|
@endophage Possibly we can move the expiry and mark active functionality to the signer, if you want to eliminate the table. So any key created on the signer will be marked for automatic deletion, and whenever we sign in new keys we compare the old keys on the root to the new keys on the root, and tell the signer to delete any old unused keys. This doesn't prevent someone from just spamming the rotate endpoint though. We can put an in-memory rate limiter in all the servers, but then we run into the inconsistency if someone load balances a bunch of servers together. In a sense, the "timestamp_keys" table (yes, unfortunately named) is just a shared cache of rate-limiting info. |
|
Yeah, I'm wracking my brain trying to come up with a better way to handle the whole pending aspect. I'm almost on the side of rotating server keys requires an immediate publish. It already has to be an online operation, maybe we should just force the new key to be published immediately. |
|
@endophage As in we immediately delete the key that's there and create a new one every time rotate is called? So nothing else can be signed with the old key? |
|
I was thinking more we can do the comparison between old and new root file and delete based on that. It still leaves us open to abuse from people that write their own client, but won't allow somebody to abuse us (at least, not in the sense they can create lots of pending keys that get left sitting around) by just calling |
|
@endophage Can't they just curl the endpoint, over and over? |
|
@cyli yeah, if they get a token and pass it along in the curl :-/ |
|
Also, if we eliminate the table, won't we break the existing API for GET-ing a key (because we can't get the key for the role anymore?) Or do you see the GET endpoint as the rotation endpoint? |
|
@endophage Also, what do you think about a sort of combination of ideas? Comparing the old and new root file and deleting keys in the signer based on that. But also keeping a table (we can name it something else) that just has the key IDs of the pending keys, and we clear them out after a day or so? We don't even have to store the gun or role - just the IDs and expiry. Just sort of a cleanup table. Possibly not even an expiry - we can just wipe out entries older than a day or two. |
|
Closing this for now, since there is a pretty big change on how this works |
Non-database changes for allowing a the rotation of a server-managed key.
Some of this is already in #529.
In order to keep the changes small, I plan on cherry-picking the server/storage changes out and working on the database changes in another PR that should be merged before this PR.